fix(s3): preserve datetime64[us] resolution for timestamps outside na… - #3359
fix(s3): preserve datetime64[us] resolution for timestamps outside na…#3359bujjibabukatta wants to merge 5 commits into
Conversation
|
Hi @kukushking could you please review and approve pull request? |
kukushking
left a comment
There was a problem hiding this comment.
Hi, unfortunately this change breaks backwards-compatibility beyond fixing the bug (timestamp overflow) it intends to fix by changing three things at once, each having their own consequences:
- coerce_timestamps: "ms" → "us" is the load-bearing change for the bug fix, and the safest of the three. Fine - could land alone.
- version: "1.0" → "2.6" - necessary because typed timestamps need 2.x logical types. It is defensible, however we need to document the EMR/legacy-Spark compatibility hit and consider gating with a deprecation path: keep 1.0 default for one release, emit a DeprecationWarning, flip in the next major.
- flavor: "spark" → None is not required Keep flavor="spark" (or document explicitly that this is a separate, intentional behavior change about column-name sanitization).
…lity per reviewer feedback
|
Hi @kukushking Thanks for the review! Reverted flavor back to "spark" and version back to "1.0", keeping only the coerce_timestamps "ms" → "us" change as the actual bug fix. Let me know if this looks good! |
|
Hi @kukushking I have updated code based on your comments. Please approve pull request? |
|
Hi @kukushking, @robert-schmidtke could you please review and approve pull request? |
robert-schmidtke
left a comment
There was a problem hiding this comment.
Honestly I am not sure about the fix. Just using microseconds instead of nanoseconds unconditionally feels like it could be breaking to environments that use pandas 2.x.
As I am merely a reporter of the bug I am not really in a position to give a go/no go on this PR. At work I would suggest to add some test cases (maybe across different pandas versions even) to ensure the timestamps are preserved correctly.
| return pa.timestamp(unit="s") | ||
| else: | ||
| return pa.timestamp(unit="ns") | ||
| return pa.timestamp(unit="us") # pandas 3.0 default is us not ns |
There was a problem hiding this comment.
Shouldn't this return nanoseconds or microseconds based on the actual pandas version installed? Otherwise this would break with pandas 2.x, right?
|
Hi @kukushking @robert-schmidtke addressed both: kept flavor="spark" and Also fixed the pandas-version assumption in _data_types.py: it now checks |
Closes #3357
Problem
pandas 3.0 defaults timestamps to
datetime64[us]resolution. When writingparquet files containing timestamps outside the nanosecond range (before 1677
or after 2262), data was silently corrupted on read-back.
Root cause: the write defaults
flavor="spark"andversion="1.0"ins3/_write_parquet.pyforce all timestamps to INT96 physical storage(nanosecond-only). Coercing
datetime64[us]values outside the nanosecondrange into INT96 overflows, producing garbage timestamps on read.
Changes
awswrangler/s3/_write_parquet.pycoerce_timestamps:"ms"→"us"— prevents microsecond precision lossflavor:"spark"→None— removes forced INT96 physical storageversion:"1.0"→"2.6"— uses typed timestamps instead of INT96awswrangler/_data_types.pypyarrow2pandas_defaults: addcoerce_temporal_nanoseconds=False— preventspyarrow from casting
timestamp[us]back todatetime64[ns]on readathena2pyarrow: map all pandas datetime resolutions (ns/us/ms/s) to thecorrect pyarrow timestamp unit; default to
usfor pandas 3.0+dtype in ("binary" or "varbinary")→dtype in ("binary", "varbinary")—"binary" or "varbinary"evaluatedto
"binary"only, causingvarbinaryto raiseUnsupportedTypeVerification